refactor(versionmap): Version-only keys, Mapping API, and iter_pairs by vshawrh · Pull Request #997 · python-wheel-build/fromager

vshawrh · 2026-03-30T20:03:11Z

What

Restrict VersionMap to packaging.version.Version keys only (no string coercion or parsing inside the map).
Add iter_pairs() to yield (version, value) in descending version order.
Subclass collections.abc.Mapping (__getitem__, __iter__, __len__); mutations stay on add().
Use iter_pairs() in VersionMapProvider.find_candidates.
Update tests to use Version(...) keys and cover invalid keys plus basic Mapping behavior.

Why

Implements the follow-up from #986: clearer API (callers own version parsing), a single iterator for version + associated data (e.g. URL), and a conventional read-only mapping interface without implying dict-style assignment.

Breaking change: code that constructed VersionMap with string keys must switch to Version("…").

Closes #986

coderabbitai · 2026-03-30T20:03:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 31887f47-e8c6-4b7f-a4e4-fd71f8f5086c

📥 Commits

Reviewing files that changed from the base of the PR and between 006ca3f and 9d203c7.

📒 Files selected for processing (4)

src/fromager/resolver.py
src/fromager/versionmap.py
tests/test_resolver.py
tests/test_versionmap.py

🚧 Files skipped from review as they are similar to previous changes (4)

src/fromager/resolver.py
tests/test_resolver.py
src/fromager/versionmap.py
tests/test_versionmap.py

📝 Walkthrough

Walkthrough

The PR refactors VersionMap to implement collections.abc.Mapping[Version, Any], adding iter and len and changing versions() to return an iterator. Initialization, add, and getitem now require packaging.version.Version keys and raise TypeError for non-Version keys; internal string-to-Version conversion was removed. VersionMap iteration yields versions in descending order. Resolver.find_candidates() was updated to iterate over the VersionMap via items() (producing (version, url) pairs) instead of indexing per-version. Tests were updated to use Version keys and to assert the new type and mapping behaviors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main changes: restricting VersionMap to Version-only keys, implementing Mapping API, and adding iter_pairs.
Description check	✅ Passed	The description clearly outlines what changed, why (referencing issue `#986`), and the breaking change regarding string keys.
Linked Issues check	✅ Passed	All three objectives from issue `#986` are met: Version-only keys enforced, iter_pairs() added for version+data iteration, and Mapping ABC subclassed.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the three objectives: VersionMap refactoring, resolver integration, and corresponding test updates.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

tiran · 2026-04-01T05:30:21Z

+        """
+        for version in self.versions():
+            yield version, self._content[version]
+


Is it necessary to introduce a new function? The items() method from Mapping ABC already returns key/value pairs.

I agree with @tiran here.

rd4398

This looks overall good. @vshawrh can you address the feedback so that we can get it merged?

coderabbitai

🧹 Nitpick comments (1)

src/fromager/versionmap.py (1)

11-15: ⚡ Quick win

Add a versionchanged note for the breaking key-type contract.

This docstring describes a user-facing breaking change but doesn’t include a Sphinx .. versionchanged:: directive.

Suggested doc update

 class VersionMap(Mapping[Version, typing.Any]):
     """Read-only mapping protocol over versions with helpers for resolution.
 
+    .. versionchanged:: 0.0.0
+       Keys must be :class:`packaging.version.Version` instances. String key
+       coercion/parsing was removed; callers must parse before use.
+
     Keys must be :class:`packaging.version.Version` instances; callers are
     responsible for parsing strings. Mutate the map via :meth:`add`.
     """

As per coding guidelines: "**/*.{rst,py}: Use Sphinx versionadded, versionremoved, versionchanged directives for user-facing changes".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/fromager/versionmap.py` around lines 11 - 15, Update the module docstring
in src/fromager/versionmap.py to include a Sphinx ".. versionchanged::"
directive documenting the breaking key-type contract: state that keys are now
required to be packaging.version.Version instances (callers must parse strings)
and reference the mutation method add; include the project version (or
placeholder) after ".. versionchanged::" and a short note describing the change
and its impact on callers.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/fromager/versionmap.py`:
- Around line 11-15: Update the module docstring in src/fromager/versionmap.py
to include a Sphinx ".. versionchanged::" directive documenting the breaking
key-type contract: state that keys are now required to be
packaging.version.Version instances (callers must parse strings) and reference
the mutation method add; include the project version (or placeholder) after "..
versionchanged::" and a short note describing the change and its impact on
callers.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5749f994-fd1d-4108-a6fa-233d0b45b23d

📥 Commits

Reviewing files that changed from the base of the PR and between fc2cbfa and 006ca3f.

📒 Files selected for processing (3)

src/fromager/resolver.py
src/fromager/versionmap.py
tests/test_versionmap.py

🚧 Files skipped from review as they are similar to previous changes (1)

src/fromager/resolver.py

rd4398 · 2026-05-13T18:14:58Z

@vshawrh can we squash the commits?

- Subclass collections.abc.Mapping with Version-only keys (no str coercion) - Use Mapping.items() in VersionMapProvider.find_candidates - Reject non-Version keys with TypeError Co-Authored-By: Claude <claude@anthropic.com> Closes: python-wheel-build#986 Co-authored-by: Cursor <cursoragent@cursor.com>

vshawrh requested a review from a team as a code owner March 30, 2026 20:03

mergify Bot added the ci label Mar 30, 2026

tiran reviewed Apr 1, 2026

View reviewed changes

rd4398 reviewed Apr 29, 2026

View reviewed changes

rd4398 requested a review from LalatenduMohanty April 29, 2026 19:03

coderabbitai Bot reviewed May 12, 2026

View reviewed changes

vshawrh force-pushed the feat/versionmap-issue-986 branch from 006ca3f to 9d203c7 Compare May 13, 2026 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(versionmap): Version-only keys, Mapping API, and iter_pairs#997

refactor(versionmap): Version-only keys, Mapping API, and iter_pairs#997
vshawrh wants to merge 1 commit into
python-wheel-build:mainfrom
vshawrh:feat/versionmap-issue-986

vshawrh commented Mar 30, 2026

Uh oh!

coderabbitai Bot commented Mar 30, 2026 •

edited

Loading

Walkthrough

Estimated code review effort

Uh oh!

tiran Apr 1, 2026

Uh oh!

rd4398 Apr 29, 2026

Uh oh!

rd4398 left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

rd4398 commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

vshawrh commented Mar 30, 2026

What

Why

Uh oh!

coderabbitai Bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Uh oh!

tiran Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

rd4398 Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

rd4398 left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

rd4398 commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Mar 30, 2026 •

edited

Loading